Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Core: added .entries property to error thrown when duplicate stories are present #20038

Merged
merged 2 commits into from
Dec 2, 2022
Merged

Core: added .entries property to error thrown when duplicate stories are present #20038

merged 2 commits into from
Dec 2, 2022

Conversation

YuriScarbaci
Copy link

Issue:

What I did

As per #19094 I extended the Error thrown at StoryIndexGenerator.ts > chooseDuplicate method to include the full information from the entries being compared

How to test

  • Is this testable with Jest or Chromatic screenshots?
  • Does this need a new example in the kitchen sink apps?
  • Does this need an update to the documentation?

If your answer is yes to any of these, please make sure to include it in your PR.

Extra info

I tried running the process as per https://github.com/storybookjs/storybook/blob/next/CONTRIBUTING.md and yarn start is failing

Failed tasks:
   
   - @storybook/theming:prep

   See Nx Cloud run details at https://nx.app/runs/uGNl2rIURN

    at makeError (file:///([path/to/my/local/fork/escaped]/storybook/scripts/node_modules/execa/lib/error.js:59:11)
    at handlePromise (file:///([path/to/my/local/fork/escaped]/storybook/scripts/node_modules/execa/index.js:119:26)
    at processTicksAndRejections (node:internal/process/task_queues:96:5)
    at async exec ([path/to/my/local/fork]/storybook/scripts/utils/exec.ts:58:7)
    at async runTask ([path/to/my/local/fork]/storybook/scripts/task.ts:286:24)
    at async run ([path/to/my/local/fork]/storybook/scripts/task.ts:420:28) {
  shortMessage: 'Command failed with exit code 1: nx run-many --target="prep" --all --parallel --exclude=@storybook/addon-storyshots,@storybook/addon-storyshots-puppeteer -- --reset',
  command: 'nx run-many --target="prep" --all --parallel --exclude=@storybook/addon-storyshots,@storybook/addon-storyshots-puppeteer -- --reset',
  escapedCommand: 'nx run-many "--target=\\"prep\\"" --all --parallel "--exclude=@storybook/addon-storyshots,@storybook/addon-storyshots-puppeteer" -- --reset',
  exitCode: 1,
  signal: undefined,
  signalDescription: undefined,

also, since this failed, I could not run yarn test either

This PR is pretty straightforward couple liners anyway, so maybe we don't need those steps?

@yuri-scarbaci-lenio
feat: core-server:: added .entries property to the StoryIndexGenerato…
281a381 
…r>choseDuplicate method error thrown when duplicate stories are present
@yuri-scarbaci-lenio yuri-scarbaci-lenio mentioned this pull request Nov 30, 2022
Open
@tmeasday
Copy link
Member

tmeasday commented Dec 1, 2022

Hey @yuri-scarbaci-lenio looks like maybe you need to re-install in code/ perhaps.

I'm not sure attaching that extra field to the error will show up the node console will it?

@yuri-scarbaci-lenio
Copy link
Contributor

Hello @tmeasday

I tested plain unhandled error throwing in node 16 and the extra information is actually printed in my installation,
After your comment I double checked on stack-blitz just to confirm and it looks like it's also being showed
image

https://stackblitz.com/edit/vitejs-vite-97fard?file=src%2Fmain.jsx,src%2FApp.jsx,package.json,throwError.js&terminal=dev

am I missing some sort of error handling that storybook core-server is doing under the hood that I should look out for?

@yuri-scarbaci-lenio
Copy link
Contributor

also, regarding why the contribuiting.md is failing I have some news
image

looks like the scripts that are being run doesn't support folder names including spaces
so in my case I have the following path
/Users/yuri.scarbaci/Desktop/personal github/storybook and it's causing some script to look for /Users/yuri.scarbaci/Desktop/personal instead 🤔

@yuri-scarbaci-lenio
Copy link
Contributor

Since I got atleast yarn test to work by renaming my folders to not contain white spaces I added a unit test and created a custom error class for duplicated entries since it's better practice to handle it that way instead of relying on new Error being an object...

@tmeasday

regarding wheter or not the entries will get logged it all dependes on the logging mechanism,
unhandle execptions, at least for as far as I could test, do print all the properties along the .trace and .message of an Error object

If Storybook does have a custom logger that will instead only log the .trace and .message let me know how would you expect the .message to look like to provide meaningful informations ( imho, at the very least the import path of both entries is a must-have to make the error any useful, but in general I think that a project as big as storybook should have a dedicated error handling layer that allows for extra information in a non string format... )

tmeasday
tmeasday approved these changes Dec 2, 2022
View reviewed changes
Copy link
Member

@tmeasday tmeasday left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@tmeasday
Copy link
Member

tmeasday commented Dec 2, 2022

Oh, nice pickup @YuriScarbaci. Perhaps you could open an issue about the space thing and someone from the community could take a look at it.

Thanks for this PR, it is great! If the logging is working for you I think we are good.

@tmeasday tmeasday changed the title feat: core-server:: added .entries property to error thrown when duplicate stories are present Core: added .entries property to error thrown when duplicate stories are present Dec 2, 2022
@tmeasday tmeasday added maintenance User-facing maintenance tasks core labels Dec 2, 2022
@tmeasday tmeasday merged commit c244467 into storybookjs:next Dec 2, 2022
@tmeasday tmeasday mentioned this pull request Feb 16, 2023
Merged
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tmeasday tmeasday tmeasday approved these changes

Assignees
No one assigned
Labels
core maintenance User-facing maintenance tasks
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@YuriScarbaci @tmeasday @yuri-scarbaci-lenio